Skip to content

feat: add cycle estimation flag for valgrind#412

Merged
not-matthias merged 1 commit into
mainfrom
cod-2842-runner-enable-cycle-estimation-for-valgrind
Jun 19, 2026
Merged

feat: add cycle estimation flag for valgrind#412
not-matthias merged 1 commit into
mainfrom
cod-2842-runner-enable-cycle-estimation-for-valgrind

Conversation

@not-matthias

@not-matthias not-matthias commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

No description provided.

@codspeed-hq

codspeed-hq Bot commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 7 untouched benchmarks

Comparing cod-2842-runner-enable-cycle-estimation-for-valgrind (7b4f51c) with main (612399d)

Open in CodSpeed

@not-matthias not-matthias force-pushed the cod-2842-runner-enable-cycle-estimation-for-valgrind branch from 34e3357 to 40db78c Compare June 19, 2026 09:30
@not-matthias not-matthias marked this pull request as ready for review June 19, 2026 09:41
@greptile-apps

greptile-apps Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Greptile Summary

This PR adds a new --cycle-estimation experimental CLI flag (and matching CODSPEED_CYCLE_ESTIMATION env var) that, when set, passes --cycle-estimation=yes to Callgrind during simulation runs. The flag is correctly wired end-to-end and only injected inside the SimulationTool::Callgrind match arm. The semantic question is whether --cycle-estimation=yes actually changes Callgrind behaviour when --cache-sim=yes is already active — in standard Callgrind, cycle estimation is already on by default in that configuration.

Confidence Score: 4/5

Safe to merge with a clarification on flag intent — the core wiring is correct and Callgrind-scoped, but the flag may not change observable behaviour without a matching default-off override.

All propagation paths are correct and the Callgrind-only scoping is properly implemented. The concern is that --cycle-estimation=yes is Callgrind's own default when cache simulation is active, so the flag may be a no-op unless the Valgrind fork or another config layer disables cycle estimation by default.

src/executor/valgrind/measure.rs — the --cycle-estimation=yes injection should be verified against the actual default in the team's Callgrind build.

Important Files Changed

Filename Overview
src/cli/experimental.rs Adds cycle_estimation: bool arg with long, default_value_t = false, env var, and display in active_flags(). No action override — matches the existing experimental_fair_sched style.
src/executor/config.rs Adds cycle_estimation: bool field to both OrchestratorConfig and ExecutorConfig, propagates it in executor_config_for_command, and defaults it to false in the test helper. Complete and consistent propagation.
src/executor/valgrind/measure.rs Adds --cycle-estimation=yes inside the Callgrind arm only (correctly scoped). However, --cycle-estimation=yes is already Callgrind's default when --cache-sim=yes is active, so the flag may be semantically a no-op.
src/cli/exec/mod.rs Trivial one-liner that wires cycle_estimation from shared experimental args into the orchestrator config for the exec subcommand.
src/cli/run/mod.rs Mirrors the exec wiring for the run subcommand; also defaults cycle_estimation: false in the test-helper constructor.

Flowchart

 
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["CLI --cycle-estimation / CODSPEED_CYCLE_ESTIMATION"] --> B["ExperimentalArgs.cycle_estimation: bool"]
    B --> C["OrchestratorConfig.cycle_estimation"]
    C --> D["ExecutorConfig.cycle_estimation\n(via executor_config_for_command)"]
    D --> E{SimulationTool?}
    E -- Callgrind --> F["push --cycle-estimation=yes"]
    E -- Tracegrind --> G["(skipped)"]
    F --> H["valgrind ...callgrind args..."]
Loading 
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["CLI --cycle-estimation / CODSPEED_CYCLE_ESTIMATION"] --> B["ExperimentalArgs.cycle_estimation: bool"]
    B --> C["OrchestratorConfig.cycle_estimation"]
    C --> D["ExecutorConfig.cycle_estimation\n(via executor_config_for_command)"]
    D --> E{SimulationTool?}
    E -- Callgrind --> F["push --cycle-estimation=yes"]
    E -- Tracegrind --> G["(skipped)"]
    F --> H["valgrind ...callgrind args..."]
Loading

Reviews (2): Last reviewed commit: "feat(runner): add experimental --cycle-e..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed Jun 19, 2026
View reviewed changes
Comment thread src/cli/experimental.rs
Comment thread src/executor/valgrind/measure.rs Outdated
GuillaumeLagrange
GuillaumeLagrange approved these changes Jun 19, 2026
View reviewed changes

@GuillaumeLagrange GuillaumeLagrange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olgtm

@not-matthias not-matthias force-pushed the cod-2842-runner-enable-cycle-estimation-for-valgrind branch from 40db78c to 7b4f51c Compare June 19, 2026 15:40
@not-matthias not-matthias merged commit 7b4f51c into main Jun 19, 2026
22 checks passed
@not-matthias not-matthias deleted the cod-2842-runner-enable-cycle-estimation-for-valgrind branch June 19, 2026 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@GuillaumeLagrange GuillaumeLagrange GuillaumeLagrange approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@not-matthias @GuillaumeLagrange